Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a problem with datetime format in rule contents #63

Merged
merged 1 commit into from
Jun 25, 2020
Merged

Fix a problem with datetime format in rule contents #63

merged 1 commit into from
Jun 25, 2020

Conversation

joselsegura
Copy link
Collaborator

Description

Added a new way to parse the datetime format in order to allow the 2 different ones observed (with and without timezone)

Fixes #60

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Testing steps

Regular CI

@joselsegura
Copy link
Collaborator Author

CI is reporting in govet:

content/export_test.go:19:20: variable declaration copies lock value to Storage: github.com/RedHatInsights/insights-results-smart-proxy/content.RulesWithContentStorage

Anyone knows how to solve it? I don't find any other way to check if the rule content is properly parsed than this one in the test case...

tisnik
tisnik approved these changes Jun 24, 2020
View reviewed changes
Copy link
Collaborator

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 🔫

Serhii1011010
Serhii1011010 approved these changes Jun 24, 2020
View reviewed changes
Copy link

@Serhii1011010 Serhii1011010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great

natiiix
natiiix approved these changes Jun 25, 2020
View reviewed changes
Copy link

@natiiix natiiix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bond95
bond95 suggested changes Jun 25, 2020
View reviewed changes
content/content_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #63 into master will increase coverage by 3.42%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   33.49%   36.92%   +3.42%     
==========================================
  Files          10       12       +2     
  Lines         627      734     +107     
==========================================
+ Hits          210      271      +61     
- Misses        393      436      +43     
- Partials       24       27       +3
Impacted Files Coverage Δ
content/parsing.go 65.51% <58.33%> (ø)
content/content.go 46.93% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca4176f...558cff5. Read the comment docs.

@joselsegura joselsegura merged commit fb93074 into RedHatInsights:master Jun 25, 2020
@joselsegura joselsegura deleted the fix_time_parse branch June 25, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Serhii1011010 Serhii1011010 Serhii1011010 approved these changes

@natiiix natiiix natiiix approved these changes

@tisnik tisnik tisnik approved these changes

@bond95 bond95 bond95 approved these changes

@quarckster quarckster quarckster approved these changes

@Bee-lee Bee-lee Awaiting requested review from Bee-lee

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Time parsing error
7 participants
@joselsegura @codecov-commenter @quarckster @bond95 @tisnik @natiiix @Serhii1011010